Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MaybeUninit for storage of inline items #162

Merged
merged 2 commits into from
Oct 19, 2019

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Sep 8, 2019

Fixes #126

I didn't see a PR open for this, and was poking around so I figured I'd do it. Sorry if I stepped on anyone's toes.

This should remove the use of mem::uninitialized, and ensure that all values used directly or via references are initialized. (e.g. T and &T are always initialized, but *const T and *mut T may not be, which is fine).

With these changes, miri passes. I also changed the CI to run miri, hopefully.

That said, this includes three breaking changes, so it will require a min version bump.

  1. The functions on the Array trait ptr and ptr_mut have been removed. Because these took a &self/&mut self argument, there's no way for us to call them when we only have a MaybeUninit<A>. Now, we just use the memory of the object directly.

    This limits the flexibility of custom implementations of Array, as they can no longer return pointers to values other than themselves, but hopefully nobody does that (IMO there's a pretty good case for sealing Array...)

  2. SmallVec::from_buf_and_len_unchecked now takes a MaybeUninit and not an A. This means that callers have the option of only partially initializing said array without risking nasal demons. I can undo this change if desired, as it's

  3. Rust 1.36.0 is required to use MaybeUninit, so this bumps the MSRV from 1.20.0 to 1.36.0. It seems tricky to make the use of MaybeUninit optional here, at least without a bunch or work (ignoring the fact that doing so would require adding a build.rs which parses rustc -Vv to tell us the version).


This change is Reviewable

lib.rs Outdated Show resolved Hide resolved
@Shnatsel
Copy link

Shnatsel commented Sep 8, 2019

If you're bumping MSRV, you might as well get rid of hacks such as #158

@thomcc
Copy link
Contributor Author

thomcc commented Sep 8, 2019

I saw that but didn't want to change too many unrelated things in this PR. I'm willing to make that change too if desired though.

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this makes SmallVec<[T; size]> use MaybeUninit<[T; size]>, shouldn't it conceptually use [MaybeUninit<T>; size]?

Anyhow this looks fine, with the Cargo.lock updated to account for the new minimum rust version change.

lib.rs Outdated
// this check should be optimized away entirely for valid ones.
assert!(
(mem::size_of::<A>() == A::size() * mem::size_of::<A::Item>()) &&
(mem::align_of::<A>() >= mem::align_of::<A::Item>())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Parens here are redundant.

KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019
This is designed to merged after servo#162, as it deprecates
a function that should be no longer necessary on newer
Rust versions.
KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019
This is designed to merged after servo#162, as it deprecates
a function that should be no longer necessary on newer
Rust versions.
KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019
This is designed to merged after servo#162, as it deprecates
a function that should be no longer necessary on newer
Rust versions.
KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019
This is designed to merged after servo#162, as it deprecates
a function that should be no longer necessary on newer
Rust versions.
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #167) made this pull request unmergeable. Please resolve the merge conflicts.

@jonhoo
Copy link

jonhoo commented Oct 16, 2019

This would be a good change to land given the new mem::unitialized lint in 1.38.

@jonhoo
Copy link

jonhoo commented Oct 18, 2019

@thomcc Do you have some spare cycles to look at the merge conflict?
@jdm / @emilio Is this something you'd consider merging soon? Currently, running miri on anything that depends on SmallVec gives

error[E0080]: Miri evaluation error: type validation failed: encountered undefined address in pointer at [0]
   --> /home/jon/.cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-0.6.10/lib.rs:410:49
    |
410 |                 data: SmallVecData::from_inline(mem::uninitialized()),
    |                                                 ^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: type validation failed: encountered undefined address in pointer at [0]
    |
    = note: inside call to `smallvec::SmallVec::<[&str; 1]>::new` at /home/jon/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:227:5

@thomcc
Copy link
Contributor Author

thomcc commented Oct 18, 2019

@thomcc Do you have some spare cycles to look at the merge conflict?

Ah, right, thanks for the prod. Sorry about the delay, I'll update it tonight.

Thom Chiovoloni added 2 commits October 18, 2019 11:22
This includes two breaking changes, in addition to the fact that it will
require a MSRV bump:

1. The functions on the `Array` trait `ptr` and `ptr_mut` have been
   removed. Because these took a `&self`/`&mut self` argument, there's
   no way for us to call them when we only have a `MaybeUninit<A>`. Now,
   we just use the memory of the object directly.

   This limits the flexibility of custom implementations of `Array`,
   (they can no longer return pointers to values other than themselves)
   but I imagine this is very rare and was probably broken somehow to
   begin with. Anybody who does this will get a compile error.

2. `from_buf_and_len_unchecked` now takes a MaybeUninit<A>, so that
   callers have the option of only partially initializing the array.
@thomcc
Copy link
Contributor Author

thomcc commented Oct 18, 2019

So this makes SmallVec<[T; size]> use MaybeUninit<[T; size]>, shouldn't it conceptually use [MaybeUninit; size]?

IIUC there's no difference in practice. And this is less of a change to the API surface, and less work to implement. But yeah... possibly?

Anyhow this looks fine, with the Cargo.lock updated to account for the new minimum rust version change.

Not sure what you mean by this. This repository has Cargo.lock in it's .gitignore.

@emilio
Copy link
Member

emilio commented Oct 19, 2019

I meant the Cargo.toml version, sorry. But I guess we can just update it later if you want.

Anyhow this looks good.

@thomcc
Copy link
Contributor Author

thomcc commented Oct 19, 2019

I've rebased the branch so that it's up to date with the latest master, but I'm... unsure what github is still upset about regarding conflicts. 😅 It even says on master...thomcc:maybe-uninit that it's able to be merged... Maybe it will sort itself out in a bit...

@emilio
Copy link
Member

emilio commented Oct 19, 2019

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 28c09eb has been approved by emilio

bors-servo pushed a commit that referenced this pull request Oct 19, 2019
Use MaybeUninit for storage of inline items

Fixes #126

I didn't see a PR open for this, and was poking around so I figured I'd do it. Sorry if I stepped on anyone's toes.

This should remove the use of mem::uninitialized, and ensure that all values used directly or via references are initialized. (e.g. T and &T are always initialized, but `*const T` and `*mut T` may not be, which is fine).

With these changes, `miri` passes. I also changed the CI to run `miri`, hopefully.

That said, this includes three breaking changes, so it will require a min version bump.

1. The functions on the `Array` trait `ptr` and `ptr_mut` have been removed. Because these took a `&self`/`&mut self` argument, there's no way for us to call them when we only have a `MaybeUninit<A>`. Now, we just use the memory of the object directly.

    This limits the flexibility of custom implementations of `Array`, as they can no longer return pointers to values other than themselves, but hopefully nobody does that (IMO there's a pretty good case for sealing `Array`...)

2. `SmallVec::from_buf_and_len_unchecked` now takes a MaybeUninit<A> and not an A. This means that callers have the option of only partially initializing said array without risking nasal demons. I can undo this change if desired, as it's

3. Rust 1.36.0 is required to use MaybeUninit, so this bumps the MSRV from 1.20.0 to 1.36.0. It seems tricky to make the use of MaybeUninit optional here, at least without a bunch or work (ignoring the fact that doing so would require adding a build.rs which parses `rustc -Vv` to tell us the version).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/162)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Testing commit 28c09eb with merge 690d65e...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: emilio
Pushing 690d65e to master...

@bors-servo bors-servo merged commit 28c09eb into servo:master Oct 19, 2019
@bors-servo bors-servo mentioned this pull request Oct 19, 2019
emilio pushed a commit that referenced this pull request Oct 19, 2019
This is designed to merged after #162, as it deprecates
a function that should be no longer necessary on newer
Rust versions.
@@ -11,4 +11,5 @@ script: |
([ $TRAVIS_RUST_VERSION != nightly ] || cargo check --verbose --no-default-features) &&
([ $TRAVIS_RUST_VERSION != nightly ] || cargo test --verbose --features union) &&
([ $TRAVIS_RUST_VERSION != nightly ] || cargo test --verbose --all-features) &&
([ $TRAVIS_RUST_VERSION != nightly ] || cargo bench --verbose bench)
([ $TRAVIS_RUST_VERSION != nightly ] || cargo bench --verbose bench) &&
([ $TRAVIS_RUST_VERSION != nightly ] || bash ./scripts/run_miri.sh)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome -- I had adding Miri here on my TODO list but didn't get around to actually do that yet. :)

@jonhoo
Copy link

jonhoo commented Oct 26, 2019

Any chance we could do a minor release with this too so downstream projects can run miri again? :)

@mbrubeck
Copy link
Collaborator

mbrubeck commented Oct 28, 2019

I would prefer to do this in the next major release, because it increases our minimum Rust version to 1.36 (from 1.20 in the previous release). Although a Rust version increase is not a breaking change at the source level, it still causes problems for downstream crates when it happens in a minor update to a widely-used library.

#73 is tracking the breaking changes going into the next major release. I'll try to get the last items merged in the next few days, and then publish the release.

dpc pushed a commit to dpc/rust-smallvec that referenced this pull request Oct 30, 2019
This is designed to merged after servo#162, as it deprecates
a function that should be no longer necessary on newer
Rust versions.
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Oct 30, 2019
* [breaking change] Use `MaybeUninit` internally to avoid possible undefined behavior (servo#162, servo#170).
* [breaking change] The `drain` method now takes a range argument, just like the standard `Vec::drain` (servo#145).
* [breaking change] Remove the `unreachable` function and replace it with the new standard `unreachable_unchecked` function (servo#164).
* [breaking change] Use `no_std` by default. This crate depends only on `core` and `alloc` by default. If the optional `write` feature is enabled then it depends on `std` so that `SmallVec<[u8, _]>` can implement the `std::io::Write` trait (servo#173).
* Add support for 96-element small vectors, `SmallVec<[T; 96]>` (servo#163).
* Iterators now implement `FusedIterator` (servo#172).
* Indexing now uses the standard `SliceIndex` trait (servo#166).
* Remove the deprecated `VecLike` trait (servo#165).
* Use `NonNull` internally (servo#171).
* Add automatic fuzz testing and MIRI testing (servo#168, servo#162).
* Update syntax and formatting to Rust 2018 standard (servo#174, servo#167).
@mbrubeck mbrubeck mentioned this pull request Oct 30, 2019
bors-servo pushed a commit that referenced this pull request Nov 2, 2019
Version 1.0.0

* Requires Rust 1.36 or later.
* [breaking change] Use `MaybeUninit` to avoid possible undefined behavior (#162, #170).
* [breaking change] The `drain` method now takes a range argument, just like the standard `Vec::drain` (#145).
* [breaking change] Remove the `unreachable` function and replace it with the new standard `unreachable_unchecked` function (#164).
* [breaking change] Use `no_std` by default. This crate depends only on `core` and `alloc` by default. If the optional `write` feature is enabled then it depends on `std` so that `SmallVec<[u8;_]>` can implement the `std::io::Write` trait (#173).
* Add support for 96-element small vectors, `SmallVec<[T; 96]>` (#163).
* Iterators now implement `FusedIterator` (#172).
* Indexing now uses the standard `SliceIndex` trait (#166).
* Remove the deprecated `VecLike` trait (#165).
* Use `NonNull` internally (#171).
* Add automatic fuzz testing and MIRI testing (#168, #162).
* Update syntax and formatting to Rust 2018 standard (#174, #167).
@RalfJung
Copy link
Contributor

RalfJung commented Nov 4, 2019

When rust-lang/rust#66059 lands, many users of the 0.6 series will see their programs break because of the incorrect mem::uninitialized. Is there any way a variant of this patch could land on the 0.6 branch so that getting the fix to them doesn't require a major version bump?

@RalfJung
Copy link
Contributor

RalfJung commented Nov 4, 2019

If the concern is the MSRV bump, one option is to use https://crates.io/crates/maybe-uninit: for Rust versions prior to 1.36, that's not actually fixing the UB, but it is certainly no worse than what smallvec currently does. And for newer Rust versions, the UB is fixed.

mbrubeck pushed a commit to mbrubeck/rust-smallvec that referenced this pull request Nov 4, 2019
This is a backport of servo#162 to the smallvec 0.6 branch. To avoid bumping the
minimum Rust version, the `maybe-uninit` crate is used in place of
`std::mem::MaybeUninit`.  To avoid breaking changes, the `Array::ptr` and
`ptr_mut` methods are retained but are no longer used, and the API to
`from_buf_and_len_unchecked` is unchanged.
@mbrubeck
Copy link
Collaborator

mbrubeck commented Nov 4, 2019

Submitted #180 to backport the MaybeUninit changes to 0.6 using the maybe-uninit crate.

mbrubeck pushed a commit to mbrubeck/rust-smallvec that referenced this pull request Nov 4, 2019
This is a backport of servo#162 to the smallvec 0.6 branch. To avoid bumping the
minimum Rust version, the `maybe-uninit` crate is used in place of
`std::mem::MaybeUninit`.  To avoid breaking changes, the `Array::ptr` and
`ptr_mut` methods are retained but are no longer used, and the API to
`from_buf_and_len_unchecked` is unchanged.
Centril added a commit to Centril/rust that referenced this pull request Nov 6, 2019
bump smallvec to 1.0

This includes servo/rust-smallvec#162, fixing an unsoundness in smallvec.

See servo/rust-smallvec#175 for the 1.0 release announcement.

Cc @mbrubeck @emilio
@thomcc thomcc deleted the maybe-uninit branch January 29, 2020 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible UB from use of uninitialized [&T; N]
9 participants